Removes SourceFilesConfiguration in favor of list of LayerConfiguration in BuildConfiguration. #516

coollog · 2018-07-06T21:33:19Z

This is also in preparation for Jib as a library for Java.

This also fixes #622, fixes #563, and moves entrypoint to be provided to BuildConfiguration (removing mainClass and jvmFlags).

…on in BuildConfiguration.

loosebazooka · 2018-07-11T15:30:25Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildSteps.java

-  private static ImmutableList<String> getEntrypoint(
-      BuildConfiguration buildConfiguration, SourceFilesConfiguration sourceFilesConfiguration) {
+  private static ImmutableList<String> getEntrypoint(BuildConfiguration buildConfiguration) {
+    // TODO: Don't use indexes.


Oh, yeah this is a weird problem.

Would it be weird to split it into java and non-java layer groups?

coollog · 2018-07-19T21:03:56Z

Fixed a bunch of merge conflicts - still need to make the changes in jib-gradle-plugin and jib-maven-plugin

coollog · 2018-07-20T18:38:10Z

This should be good for review now - this ended up being quite large - I'll do a pass myself too

chanseokoh

Briefly went through 1/3 of the code. Will look into it further.

chanseokoh · 2018-07-23T14:15:33Z

.../src/integration-test/java/com/google/cloud/tools/jib/builder/BuildStepsIntegrationTest.java

-  @Rule public TemporaryFolder temporaryCacheDirectory = new TemporaryFolder();
-
-  @Rule public TemporaryFolder temporaryTarOutput = new TemporaryFolder();
+  @Rule public TemporaryFolder temporaryFolder = new TemporaryFolder();


I guess it's harmless to have final. Is the repo generally lenient about final?

We should definitely have final where-ever applicable and have things be non-final unless absolutely necessary.

chanseokoh · 2018-07-23T14:31:21Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

+     * @param entrypoint the tokenized command to run when the container starts
+     * @return this
+     */
+    public Builder setEntrypoint(@Nullable List<String> entrypoint) {


What is the point of allowing null? Also, I think we should document the behavior: "passing null does nothing; it does not clear the previous value."

The point of allowing null is to allow for building an image without an entrypoint (since it's an optional field on the image format). This can also be useful for when we propagate base image container configuration so fields like entrypoint could be propagated. But yes, we should defintely document the behavior.

If so, why not just clear the entrypoint if null is given? That's more intuitive and what I'd expect normally.

That makes a lot of sense.

Or, actually - I think we should probably have the semantics be like:

null - if base image has entrypoint, propagate; otherwise, don't set an entrypoint
non-null - set entrypoint to what is given (empty would be empty entrypoint, etc.)

chanseokoh · 2018-07-23T14:41:35Z

jib-core/src/main/java/com/google/cloud/tools/jib/configuration/LayerConfiguration.java

@@ -65,16 +77,23 @@ public static Builder builder() {
  }

  private final ImmutableList<LayerEntry> layerEntries;
+  private String label;


Probably final.

chanseokoh · 2018-07-23T15:55:54Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/BuildStepsRunner.java

-          .forEach(dependencyFile -> buildLogger.info("\t\t" + dependencyFile));
+      for (LayerConfiguration layerConfiguration :
+          buildSteps.getBuildConfiguration().getLayerConfigurations()) {
+        buildLogger.info("\t" + capitalizeFirstLetter(layerConfiguration.getLabel()) + ":");


Why capitalizing the first letter for logging? Shouldn't it be ideal to print out the label as-is? Also it'd be less of a headache if we can get out of the locale business.

I mostly had this to keep the log message consistent with what it was before. We should definitely consider changing this logic to be something more configurable, but this log message will be part of the "plugins-common" rather than jib-core. There should probably be an enum from a layer type (DEPENDENCIES, CLASSES, etc.) to the intended label string and log string.

Alright, just keep in mind you'd normally consider what locale to use if the text can be something else than English, e.g., https://docs.oracle.com/javase/7/docs/api/java/lang/String.html#toUpperCase(java.util.Locale)

chanseokoh · 2018-07-23T15:59:09Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/JavaEntrypointBuilder.java

+      List<String> classpathElements, List<String> jvmFlags, String mainClass) {
+    String classpathString = String.join(":", classpathElements);
+
+    List<String> entrypointBuilder = new ArrayList<>(4 + jvmFlags.size());


Feels weird to call it Builder.

I'll change this to EntrypointConstructor (like in the jib library design).

chanseokoh · 2018-07-23T16:05:26Z

jib-core/src/test/java/com/google/cloud/tools/jib/docker/DockerContextGeneratorTest.java

-  }
+  private String expectedDependenciesPath = "/app/libs/";
+  private String expectedResourcesPath = "/app/resources/";
+  private String expectedClassesPath = "/app/classes/";


Are these suitable for static final?

chanseokoh · 2018-07-23T16:11:59Z

jib-core/src/test/resources/sampleDockerfile

@@ -4,6 +4,7 @@ COPY libs /app/libs/
 COPY snapshot-libs /app/libs/
 COPY resources /app/resources/
 COPY classes /app/classes/
+COPY root /


Just asking: does this open up the possibility that copying the files at root/ might overwrite existing files, or is it that root/ will never have a sub-directory called app/?

Yes, it opens up that possibility.

chanseokoh · 2018-07-23T16:18:49Z

jib-gradle-plugin/CHANGELOG.md

+
+### Changed
+
+- Only builds non-empty layers ([#516](https://github.com/GoogleContainerTools/jib/pull/516/files))


Is it true? I saw above

// TODO: Don't generate empty layers. /** * Creates the Docker context in {@code #targetDirectory}.

That's just for generating the Docker context; I don't think empty layers are added to the image itself in jib:build etc.

Oops forgot to remove that - it actually skips the empty layers now.

TadCordle

Halfway done, will get to the rest later.

TadCordle · 2018-07-23T16:03:41Z

.../src/integration-test/java/com/google/cloud/tools/jib/builder/BuildStepsIntegrationTest.java

@@ -42,25 +49,36 @@
 /** Integration tests for {@link BuildSteps}. */
 public class BuildStepsIntegrationTest {

+  /** Lists the files in the {@code resourcePath} resources directory. */
+  private static ImmutableList<Path> getFilesList(String resourcePath)


If this is specifically for resources maybe it should be renamed to getResourceFilesList.

TadCordle · 2018-07-23T16:14:20Z

.../src/integration-test/java/com/google/cloud/tools/jib/builder/BuildStepsIntegrationTest.java

                .build());
+    buildImageSteps.run();


Why are we running this 3 times now?

Oops, removing.

TadCordle · 2018-07-23T16:22:51Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java


      switch (errorMessages.size()) {
        case 0: // No errors
-          if (baseImageReference == null || targetImageReference == null || mainClass == null) {
+          if (baseImageReference == null || targetImageReference == null) {


Should we error if entrypoint is empty like we used to error when mainClass was null?

I'm intending it to be possibly be empty to allow for propagation of entrypoint from base images in the future (when this is used as a library).

TadCordle · 2018-07-23T16:25:24Z

jib-core/src/main/java/com/google/cloud/tools/jib/builder/BuildConfiguration.java

    private ImmutableMap<String, String> environmentMap = ImmutableMap.of();
    private ImmutableList<Port> exposedPorts = ImmutableList.of();
    private Class<? extends BuildableManifestTemplate> targetFormat = V22ManifestTemplate.class;
    @Nullable private CacheConfiguration applicationLayersCacheConfiguration;
    @Nullable private CacheConfiguration baseImageLayersCacheConfiguration;
    private boolean allowHttp = false;
-    @Nullable private LayerConfiguration extraFilesLayerConfiguration;
+    private ImmutableList<LayerConfiguration> layerConfigurations = ImmutableList.of();
+    private ImmutableList<String> entrypoint = ImmutableList.of();


This is very much a nit, so feel free to ignore, but for organization it'd be nice to put entrypoint next to javaArguments instead of at the end.

Sounds good

(This same ordering happens in a bunch of other places too, but I didn't want to overload the comments.)

Yea, maybe we can leave that to another PR that can set up a nice ordering pattern for all of these parameters.

TadCordle · 2018-07-23T16:41:17Z

jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerContextGenerator.java


  @Nullable private String baseImage;
  private List<String> jvmFlags = Collections.emptyList();
  private String mainClass = "";
  private List<String> javaArguments = Collections.emptyList();
  private List<String> exposedPorts = Collections.emptyList();

-  public DockerContextGenerator(SourceFilesConfiguration sourceFilesConfiguration) {
-    this.sourceFilesConfiguration = sourceFilesConfiguration;
+  // TODO: Just take the LayerConfigurations.


Would it be that difficult to just do this now? I think we should avoid adding TODOs while trying to fix other issues.

I'll look into that again, but the main issue is that since this is in jib-core, it doesn't know what layer label corresponds to which copy directive.

TadCordle · 2018-07-23T16:41:29Z

jib-core/src/main/java/com/google/cloud/tools/jib/docker/DockerContextGenerator.java

@@ -114,6 +180,7 @@ public DockerContextGenerator setExposedPorts(List<String> exposedPorts) {
    return this;
  }

+  // TODO: Don't generate empty layers.


TadCordle · 2018-07-23T16:47:51Z

jib-core/src/main/java/com/google/cloud/tools/jib/frontend/BuildStepsRunner.java

@@ -166,6 +156,13 @@ private static void handleRegistryUnauthorizedException(
    }
  }

+  private static String capitalizeFirstLetter(String string) {


Could use StringUtils#capitalize() instead?

I'm trying to avoid having to add the Apache Commons library just for this function, and preferring Guava over Apache Commons.

TadCordle · 2018-07-23T16:53:04Z

jib-core/src/test/java/com/google/cloud/tools/jib/builder/BuildConfigurationTest.java

@@ -198,7 +179,7 @@ public void testBuilder_missingValues() {

    } catch (IllegalStateException ex) {
      Assert.assertEquals(
-          "base image is required but not set, target image is required but not set, and main class is required but not set",


Hmm I think we should keep a test for the default case of having 3 or more error messages. Or we can just delete that default case logic since we only have two possible error messages at the moment (missing target and missing base).

TadCordle · 2018-07-24T19:21:18Z

jib-gradle-plugin/CHANGELOG.md

@@ -9,6 +9,15 @@ All notable changes to this project will be documented in this file.

 ### Fixed

+## 0.9.8


Shouldn't these still be under unreleased?

TadCordle

Other than that LGTM, should file issues for the new TODOs.

coollog · 2018-07-25T15:31:52Z

Trying to fix an integration test on this, will merge after that is fixed.

TODOs have been filed too.

coollog · 2018-07-25T15:36:35Z

Ah looks like it was just timing out when pulling the image because I am on a slow internet.

Removes SourceFilesConfiguration in favor of list of LayerConfigurati…

b468528

…on in BuildConfiguration.

Applies to jib-gradle-plugin.

69e6b57

coollog mentioned this pull request Jul 10, 2018

Adds BuildConfiguration#setExtraFilesLayerConfiguration. #480

Merged

Fixes format.

84f5ef6

coollog mentioned this pull request Jul 11, 2018

Builds the extra files layer. #489

Merged

loosebazooka reviewed Jul 11, 2018

View reviewed changes

schmitch mentioned this pull request Jul 19, 2018

split resource folder and class folder sbt-jib/sbt-jib#1

Closed

coollog mentioned this pull request Jul 19, 2018

Support WARs. #431

Closed

coollog added 4 commits July 19, 2018 16:13

Merge branch 'master' into no-sourcefilesconfiguration

fd6daaf

Merge branch 'master' into no-sourcefilesconfiguration

780332a

Fixes auth issue.

e3d835d

Merge branch 'master' into no-sourcefilesconfiguration

fd46ee2

coollog added 5 commits July 20, 2018 11:10

Merge branch 'master' into no-sourcefilesconfiguration

4736f4d

Refactors entrypoint building.

00176fe

Fixes tests.

36791ac

Fixes jib-gradle-plugin.

fcde23c

Changes for jib-maven-plugin.

06e5b46

coollog requested a review from a team July 20, 2018 18:37

coollog and others added 6 commits July 20, 2018 14:38

Merge branch 'master' into no-sourcefilesconfiguration

0bc2536

Simplifies DockerContextGenerator and labels layers to avoid indexing.

7162e85

Merge branch 'master' into no-sourcefilesconfiguration

7c28ffe

Fixes words.

e871c14

Prints all layer entries.

77339ff

Fixes format.

614dbc7

chanseokoh reviewed Jul 23, 2018

View reviewed changes

TadCordle reviewed Jul 23, 2018

View reviewed changes

coollog added 5 commits July 23, 2018 22:11

Changes fields to nullable.

5a8247a

Renames JavaEntrypointConstructor.

bc46b0b

Merge branch 'master' into no-sourcefilesconfiguration

72abd98

Some more comments.

0ee1e86

Fixes format.

3596206

TadCordle mentioned this pull request Jul 24, 2018

Move BuildConfiguration to configuration package #702

Closed

TadCordle reviewed Jul 24, 2018

View reviewed changes

TadCordle approved these changes Jul 24, 2018

View reviewed changes

coollog added 2 commits July 24, 2018 16:41

Fixes CHANGLOG.

d7da59d

A typo.

69764a0

This was referenced Jul 24, 2018

Improve BuildConfiguration parameter layout. #709

Closed

Have DockerContextGenerator take LayerConfigurations instead of LayerEntrys. #710

Closed

coollog and others added 2 commits July 24, 2018 17:08

Fixes format.

19db093

Merge branch 'master' into no-sourcefilesconfiguration

bbe18c4

coollog merged commit 6bf0148 into master Jul 25, 2018

coollog deleted the no-sourcefilesconfiguration branch July 25, 2018 15:47


		### Changed

		- Only builds non-empty layers ([#516](https://github.com/GoogleContainerTools/jib/pull/516/files))

		@@ -9,6 +9,15 @@ All notable changes to this project will be documented in this file.

		### Fixed

		## 0.9.8

Removes SourceFilesConfiguration in favor of list of LayerConfiguration in BuildConfiguration. #516

Removes SourceFilesConfiguration in favor of list of LayerConfiguration in BuildConfiguration. #516

Conversation

coollog commented Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coollog commented Jul 19, 2018

coollog commented Jul 20, 2018

chanseokoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TadCordle left a comment

Choose a reason for hiding this comment

coollog commented Jul 25, 2018 • edited Loading

coollog commented Jul 25, 2018

coollog commented Jul 6, 2018 •

edited

Loading

coollog commented Jul 25, 2018 •

edited

Loading